-
Notifications
You must be signed in to change notification settings - Fork 80
Update webhook tests following the PF5 webhook upgrade #2256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
dace442 to
fa94977
Compare
|
I have observed that the webhooks UI test can take up to 10 minutes to execute on local robottelo instance and up to 20+ minutes on PRT testing. This is with applied changes from SatelliteQE/airgun#2256 which update the test to pass after the webhook PF5 update. Without these changes, the test takes even longer. I do not think that this is a reasonable time for what the test does. I have recreated this test in foreman_webhooks: theforeman/foreman_webhooks#94 This test copies the functionality 1:1 and also tests on a live server using the Capybara framework. It takes on average 90 seconds to complete. Due to a sizable difference in a time needed for completion, I propose to replace test_webhook.py with the test in foreman_webhooks and to rely on testing in the foreman_webhooks repository when it comes to UI. This will contribute a tiny bit to reducing strain on our testing infrastructure.
|
While this is ready for review I would prefer to remove the UI webhook test completely. For more information see SatelliteQE/robottelo#20678 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- In
WebhookFormView.read, thewidget_namesparameter is currently unused; either support limiting the fields based on it or remove the parameter to avoid confusion. - In
PF5TypeaheadSelect.fill, consider usingbrowser.wait_for_element(or a similar browser helper) instead of a barewait_for+is_displayedto reduce flakiness and make the waiting logic consistent with the rest of the codebase. - The new
wait_for_popupinWebhookFormViewno longer returns a boolean like the old implementation did; if callers rely on the previous return value, either restore a boolean result or rename the method to better reflect that it only waits and does not report visibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WebhookFormView.read`, the `widget_names` parameter is currently unused; either support limiting the fields based on it or remove the parameter to avoid confusion.
- In `PF5TypeaheadSelect.fill`, consider using `browser.wait_for_element` (or a similar browser helper) instead of a bare `wait_for` + `is_displayed` to reduce flakiness and make the waiting logic consistent with the rest of the codebase.
- The new `wait_for_popup` in `WebhookFormView` no longer returns a boolean like the old implementation did; if callers rely on the previous return value, either restore a boolean result or rename the method to better reflect that it only waits and does not report visibility.
## Individual Comments
### Comment 1
<location> `airgun/widgets.py:3132-3145` </location>
<code_context>
+ Args:
+ value: The text value to select
+ """
+ input_el = self.browser.element(self.locator)
+ self.browser.clear(input_el)
+ input_el.send_keys(value)
+ option_locator = (
+ f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
+ f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Scope the option locator for `PF5TypeaheadSelect` to avoid ambiguity when multiple typeaheads are present.
Because `option_locator` is a global `//li...` XPath, `browser.is_displayed(option_locator)` / `browser.element(option_locator)` can hit the wrong dropdown when multiple PF5 typeahead selects exist on the page. Please scope the locator to an element related to `input_el` (e.g., via `aria-owns` or a closer ancestor) so it only targets the menu for this input.
```suggestion
input_el = self.browser.element(self.locator)
self.browser.clear(input_el)
input_el.send_keys(value)
# Scope the option locator to the menu associated with this input to avoid
# ambiguity when multiple PF5 typeahead selects are present.
menu_id = input_el.get_attribute("aria-owns") or input_el.get_attribute("aria-controls")
if menu_id:
option_locator = (
f'//*[@id="{menu_id}"]'
f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
)
else:
# Fallback to the global locator if no association attribute is present
option_locator = (
f'//li[contains(@class, "pf-v5-c-menu__list-item")]'
f'//span[contains(@class, "pf-v5-c-menu__item-text") and normalize-space(.)="{value}"]'
)
wait_for(
lambda: self.browser.is_displayed(option_locator),
timeout=10,
delay=0.5,
handle_exception=True,
)
self.browser.element(option_locator).click()
```
</issue_to_address>
### Comment 2
<location> `airgun/views/webhook.py:79-92` </location>
<code_context>
+ for field_name, value in tabs_to_fill[tab_name].items():
+ getattr(self, field_name).fill(value)
+
+ def read(self, widget_names=None):
+ """Read form values from all tabs."""
+ result = {'general': {}, 'credentials': {}, 'additional': {}}
+ fields = {
+ 'general': ['subscribe_to', 'name', 'target_url', 'template', 'http_method', 'enabled'],
+ 'credentials': ['user', 'password', 'verify_ssl', 'capsule_auth', 'certs'],
+ 'additional': ['content_type', 'headers'],
+ }
+ for tab_name, field_list in fields.items():
+ self._switch_to_tab(tab_name)
+ for field_name in field_list:
+ widget = getattr(self, field_name)
+ result[tab_name][field_name] = widget.read() if widget.is_displayed else None
+ return result
@property
</code_context>
<issue_to_address>
**suggestion:** Either use or remove the `widget_names` parameter in `WebhookFormView.read`.
The parameter is unused while the docstring implies configurability. To keep the API clear, either implement `widget_names`-based filtering of which fields/tabs are read, or remove the parameter if configurability isn’t required.
```suggestion
def read(self, widget_names=None):
"""Read form values from all tabs.
:param widget_names: Optional iterable of "tab_name.field_name" strings
(e.g. "general.name", "credentials.user"). When provided, only the
specified widgets will be read. When None, all known widgets are read.
"""
result = {'general': {}, 'credentials': {}, 'additional': {}}
fields = {
'general': ['subscribe_to', 'name', 'target_url', 'template', 'http_method', 'enabled'],
'credentials': ['user', 'password', 'verify_ssl', 'capsule_auth', 'certs'],
'additional': ['content_type', 'headers'],
}
allowed_widgets = None
if widget_names is not None:
# Normalize to a set of "tab.field" strings for fast membership checks
allowed_widgets = {name for name in widget_names}
for tab_name, field_list in fields.items():
if allowed_widgets is not None:
# Filter fields for this tab based on the allowed "tab.field" names
field_list = [
field_name
for field_name in field_list
if f'{tab_name}.{field_name}' in allowed_widgets
]
if not field_list:
# No requested fields for this tab; skip switching to it
continue
self._switch_to_tab(tab_name)
for field_name in field_list:
widget = getattr(self, field_name)
result[tab_name][field_name] = widget.read() if widget.is_displayed else None
return result
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
4b075eb to
365fbec
Compare
365fbec to
6cf51c8
Compare
|
LadislavVasina1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thx for the changes.
I have observed that the webhooks UI test can take up to 10 minutes to execute on local robottelo instance and up to 20+ minutes on PRT testing. This is with applied changes from SatelliteQE/airgun#2256 which update the test to pass after the webhook PF5 update. Without these changes, the test takes even longer. I do not think that this is a reasonable time for what the test does. I have recreated this test in foreman_webhooks: theforeman/foreman_webhooks#94 This test copies the functionality 1:1 and also tests on a live server using the Capybara framework. It takes on average 90 seconds to complete. Due to a sizable difference in a time needed for completion, I propose to replace test_webhook.py with the test in foreman_webhooks and to rely on testing in the foreman_webhooks repository when it comes to UI. This will contribute a tiny bit to reducing strain on our testing infrastructure.
| enabled = Checkbox(id='id-enabled') | ||
|
|
||
| # Credentials tab fields | ||
| user = TextInput(locator='//input[@id="id-user"]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(non-blocking) TextInput has also id parameter which just saves some typing than locator. Internally, it then gets translated to similar XPath anyway.
Referring theforeman/foreman_webhooks#89